-
-
Notifications
You must be signed in to change notification settings - Fork 28
Introduce CMake fetchcontent and CMake Install Workflow #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Introduce CMake fetchcontent and CMake Install Workflow #480
Conversation
56cc9e4 to
f45414e
Compare
|
@JeanPhilippeKernel @jnyfah I'm not sure why this check is failing, however, I would like to request you to try the build artifacts on your systems. The main thing with the install workflow is to move what was in the Panzerfaust/net8.0..../publish folder is not going to the bin folder with the vulkan loader library in the lib folder. This means that downloading the release builds should be able to run on a user system easily. The program doesn't run because of the "Shader::CreateDescriptorLayout()" code runs out of vram when loading up the shaders. However, I believe that the "not running on mac and linux" issue may be resolved by this. |
|
Hey @MathewBenson |
|
my first observation will be to extract the vulkan related issue in a seperate PR - and as heads-up there is an on-going effort to fix the issue here : #479 |
e7ea056 to
e007150
Compare
dependencies.cmake
Outdated
|
|
||
| target_compile_definitions(imgui PUBLIC GLFW_INCLUDE_VULKAN IMGUI_DEFINE_MATH_OPERATORS) | ||
|
|
||
| target_link_libraries(imgui PUBLIC glfw Vulkan::Headers Vulkan::Loader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is problematic ! imgui should consume Vulkan::Headers and Vulkan::Loader from zEngineLibs
imgui (and its plugin libs like imguizmo) is an optional lib for zEngineLibs.
it just helps for GUI components, from Tetragramma (The editor) and nothing more - and there is a big chance that we might want to have our internal GUI lib and drop it.
So we want the fexibility to replace it anytime
Ideally, it should be moved to Tetagramma - that will require some refactoring from the Rendering section -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As imgui and ImGuizmo do not have CMakeLists.txt files to create targets, I went with the option of creating them as separate targets as opposed to including their source files in the zEngine library.
I am using the transitive property of CMake Dependencies to pass the Vulkan and glfw from imgui -> Imguizmo -> ExternalLib ->zEngineLib. The idea was to reduce repetition.
I could make the dependency to be PRIVATE and specify them as "Build Requirements" for imgui and currently for zEngineLib.
This way imgui will have target_link_libraries(imgui PRIVATE Vulkan::Headers....
and zEngine will have target_link_libraries(External_Libs PUBLIC Vulkan::Headers.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me !
| ) | ||
|
|
||
| target_link_libraries(${TARGET_NAME} PRIVATE | ||
| zEngineLib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure i fully understand the removal here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as above the zEngineLib is transitively availed to Obelisk through Tetragrama.
| @@ -1,4 +1,5 @@ | |||
| #include <CLI/CLI.hpp> | |||
| #include <Editor.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already #include <Tetragrama/Editor.h> below
| "SPIRV_SKIP_TESTS": "ON", | ||
| "GLSLANG_ENABLE_INSTALL": "ON", | ||
| "ALLOW_EXTERNAL_SPIRV_TOOLS": "OFF", | ||
| "ALLOW_EXTERNAL_SPIRV_TOOLS": "ON", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to OFF this because we want to use the one we are compiling during the build and avoid any version mismatching with the system - any reason why this ON ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "ALLOW_EXTERNAL_SPIRV_TOOLS" is a CMake Property on GSLang that makes it to build its on SPIRV-Tools if they are not detected by CMake.
As we are already building SPIRV-Tools as an option,, we are telling it not to build its own version.
There is actually another potential repetition of this because of the step of downloading the compiler tools in the build step of the Powershell scripts. We could alleviate that download, which is like 1GB by making the shader compilation be handled as part of the CMake Processing where we use the binaries that we compile when generating GSLang Target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could alleviate that download, which is like 1GB by making the shader compilation be handled as part of the CMake Processing where we use the binaries that we compile when generating GSLang Target.
Interesting,
we can explore this in separate PR - please create an issue to track that investigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #481 to track. Coincidentally, the CI is now failing because the upstream ShaderC URLs are broken and/or the requested versions are unavailable on the server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a timing ! 😅😂
9a45dcf to
7e41a24
Compare
- CI Error when no files found - make libraries static - install tests and vulkan lib - Introduce CPack - Fix Clang-format issue moving to LLVM 21, which is now default on several platforms - simplify testing due to install workflow - Initialize SwapchainImages with the size parameter added as the creation of the swap chains was happening successfully but the size was not getting updated giving an index error. - Fix error on linux where Wayland was not being initialized properly. Wayland is the default for glfw in version 3.4 and in many desktop environments. GLFW_EXPOSE_NATIVE_* macro was defined for windows but not for the other Operating Systems
- Being worked on by another branch.
- The GLFW_NATIVE macros are required for surface creation in the respective systems. - Currently it was hard-coded for windows only. - Added the required macros for linux and macos to remove the error that was in the Logs for vk_surface not created - Also added the __externals folder to gitignore so that it is not detected. Useful when switching branches and experimenting between the older and this version.
- removal of GLM - use defined cstring - formatting on BuildEngine.ps1 - CMake Restructure for Vulkan headers and loader
7e41a24 to
5443a75
Compare
FetchContent Package Management
CMake Install
SwapChain Image Sizes
Wayland on Linux